Close underlying files when using compression - #2120

Went through this in detail — no issues found. Specifically checked:

• Close ordering: _ClosingFile.close() finalizes the compression stream first (so the trailer/CRC gets flushed) then closes the raw file in a finally, with independent if not X.closed guards on each, so a failure finalizing the compression stream (tested by test_fs_open_closes_raw_file_on_compression_error) still guarantees the raw file gets closed rather than leaking it.
• Proxy correctness: __getattr__ forwards to the wrapped stream, but __iter__/__next__/__enter__/__exit__ are all explicitly defined rather than relying on __getattr__ — correctly sidesteps the usual gotcha where Python's special-method lookup bypasses __getattr__ for dunders, so for line in f and with f: actually work through the wrapper.
• Transaction interaction: self.transaction.files.append(f) in spec.py runs before the compression wrap, so the transaction ends up tracking the raw pre-compression file object, not the _ClosingFile the caller writes through. Confirmed this still works correctly, though: _ClosingFile.close() closes that same raw object via self._raw, so transaction.files[0].closed is genuinely True after the wrapper closes, matching what test_fs_open_compression_transaction checks.
• Tests are real: reverted just the spec.py change (kept _ClosingFile itself) and reran test_compression.py — 39 of the new tests fail immediately without the fix, so this is genuine regression coverage, not decoration.

Good fix, nice test coverage (including the zstandard third-party-codec case, which has a different close-ownership shape than the stdlib gzip/bz2/lzma wrappers).

Worth flagging one snag with moving it into compress(): compress is whatever is registered in the compr registry, including third-party codecs added through register_compression, so each of those would have to do the wrapping itself to get the behaviour.

Putting it in the registry lookup instead would reach the other call sites that read compr[...] directly and already manage the raw file themselves: core.py:120, where OpenFile appends both objects to self.fobjects and closes them in reverse on exit; cached.py:386/715/941; local.py:397, where LocalFileOpener keeps self.f; and tar.py:81, where fo belongs to the caller. OpenFile would end up closing the raw file twice, and tar.py would close a file it doesn't own.

Keeping the wrapping at the AbstractFileSystem.open call site is what confines the ownership change to the one place that creates the raw file, which seems right to me.

You make a good point, but then we should also consider other filesystem-specific open() implementations that accept compression= ? Maybe there are none (because _open is usually what is used).

You're right that there are none — DirFileSystem.open is the only override, and it forwards to the wrapped filesystem, so it inherits this. Everything else, here and in s3fs/gcsfs/adlfs, only implements _open.

So the base-class call site covers everything today, and I'd keep it there. One improvement worth making: give compression.py a small helper, say open_compressed(f, compression, mode), holding the compress(...) call and the _ClosingFile wrapping, and have AbstractFileSystem.open call that. Then the ownership rule lives next to _ClosingFile rather than being spelled out at the call site, and anything that does end up needing it later is a one-liner rather than a copy of the same three lines. The other compr[...] users stay untouched.

Yes, I like moving it to compression.py

😃😃

(ping me when you've done that. I can handle the conflict if you like - it's just the changelog)

shall i go for it? happy to do it myself if @Jokasa7 doesnt, nd if u need it quick
@martindurant

no rush

Sounds good — I'll leave it with @Jokasa7 then, it's their patch.

@Jokasa7 if you'd rather not pick up the refactor, just say and I'll open a follow-up. Otherwise it's a small move: lift the compress(...) call and the _ClosingFile wrap into a helper in compression.py and have AbstractFileSystem.open call that — the other compr[...] callers stay as they are.